Add support for include_institution_data flag#134
Conversation
Supported for /institutions/search /institutions/get_by_id /institutions/get
| this.institutionId = institutionId; | ||
| } | ||
|
|
||
| public InstitutionsGetByIdRequest(String institutionId, boolean includeInstitutionData) { |
There was a problem hiding this comment.
can we add a .withIncludeInstitutionData(boolean) instead of creating more constructors?
There was a problem hiding this comment.
yes will do
| this.offset = offset; | ||
| } | ||
|
|
||
| public InstitutionsGetRequest(Integer count, Integer offset, boolean includeInstitutionData) { |
There was a problem hiding this comment.
can we add a .withIncludeInstitutionData(boolean) instead of creating more constructors?
There was a problem hiding this comment.
yes will do
| } | ||
|
|
||
|
|
||
| public InstitutionsSearchRequest withProducts(Product... products) { |
There was a problem hiding this comment.
can we add a .withIncludeInstitutionData(boolean) instead of creating more constructors?
There was a problem hiding this comment.
yes will do
There was a problem hiding this comment.
Uh. I'm not a big fan of object mutation here. Can we either make it a static factory method (ie. static method that always returns a new object)?
| return url; | ||
| } | ||
|
|
||
| public void setUrl(String url) { |
There was a problem hiding this comment.
do we need these setters?
There was a problem hiding this comment.
good catch
notthefakestephen
force-pushed
the
st-add-include-institution-data
branch
from
eb9038e to
609a4b8
Compare
|
|
||
| @Test | ||
| public void testSuccessWithIncludeInstitutionDataTrue() throws Exception { | ||
| Response<InstitutionsGetByIdResponse> response = client().service().institutionsGetById(new InstitutionsGetByIdRequest(TARTAN_BANK_INSTITUTION_ID).withIncludeInstitutionData(true)) |
There was a problem hiding this comment.
nit: can we try to keep these within the width of what github displays?
| List<AssetReportGetResponse.Account> accounts = assetReport.getItems().get(0).getAccounts(); | ||
| for (AssetReportGetResponse.Account account : accounts) { | ||
| if (account.getTransactions().size() > 0) { | ||
| return account.getTransactions().get(0).getName() != null; |
There was a problem hiding this comment.
Please confirm that a "" transaction name should return true.
Also, I can't see this method being used anywhere, what is it meant for?
There was a problem hiding this comment.
ah it was to prevent some flakiness but yes "" should return true, a report without insights will not have the name key
|
|
||
| assertSuccessResponse(response); | ||
| } | ||
|
|
There was a problem hiding this comment.
These two tests look to me like a missed opportunity to reflect what difference setting true vs false means for the result object.
Update if you have time.
3 participants
Supported for
/institutions/search
/institutions/get_by_id
/institutions/get